-
Notifications
You must be signed in to change notification settings - Fork 3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[WIP] Complex pushdown #402
Closed
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
26 tasks
martint
force-pushed
the
pushdown
branch
3 times, most recently
from
March 7, 2019 04:19
1f0b334
to
97a8ec0
Compare
More accurately, constructs a set of alternate plans with the filter pushed into the table scan based on available table layouts.
This test relies on a layout being picked during planning. For LocalQueryRunner this only happens because of a synthetic rule (PickLayout w/o predicate) that attaches a layout to the table scan. So, in a sense, it's just a coincidence that it works. In distributed execution, the job is done by AddExchanges, so we want to make sure we're testing that behavior.
It's just selecting a layout for the raw table scan, so no need to go through the logic for pushing a predicate, etc.
This change hides table layouts from the engine as a first-class concept. We keep the SPI as is for backward compatibility for now. When predicates are pushed into a table scan by PickLayout (now PushPredicateIntoTableScan) or AddExchanges, we now replace the table handle associated with the table scan with a new one that contains the reference to the ConnectorTableLayoutHandle under the covers.
It now contains a single rule, so no point in having it return a rule set.
In order to translate expression to row expressions, the code was first replacing all symbol references with field references for the corresponding ordinal inputs. This is unnecessary, as the translation can be done on demand as the expression is translated to a row expression.
The inferred type of the former expression is INTEGER, which doesn't match the signature of combineHash function call.
They were only being used in tests. The engine no longer relies on them for query execution.
There's no longer a conflict with analyzeExpressionsWithInputs so simplify the name
There's only one caller, so no need for an extra indirection.
The new class is to facilitate obtaining the type of an expression and its subexpressions during planning (i.e., when interacting with IR expression) and to remove spurious dependencies on the SQL parser. It will eventually get removed when we split the AST from the IR and we encode the type directly into IR expressions.
I guess #7994 is the successor of this PR? |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
These are the initial steps towards #18. It includes the prerequisite cleanups and refactorings (#363) and the beginnings of arbitrary filter pushdown. Just want to get it out there so that people can see where we're going with this.
Depends on #363